Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loki: Override distributor's default ring KV store #4440

Merged
merged 3 commits into from
Oct 13, 2021

Conversation

DylanGuedes
Copy link
Contributor

@DylanGuedes DylanGuedes commented Oct 8, 2021

What this PR does / why we need it:
Changes the default distributor ring KV store from consul to inmemory. This change makes the Distributor easier to run with default configs by not requiring Consul.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:

  • This blocks this PR.
  • The default flag value is originally defined by grafana/dskit so another option would be to change the flag there. However, the impact would be much higher so we preferred to override the flag for Loki only.

Checklist

  • Documentation added
  • Tests updated

@DylanGuedes DylanGuedes changed the title Loki: Override default ring KV store for the distributor Loki: Override distributor's default ring KV store Oct 8, 2021
@DylanGuedes DylanGuedes force-pushed the use-inmemory-as-default-kvstore branch from e068f84 to e5045f9 Compare October 8, 2021 19:57
Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DylanGuedes this looks good, will conflict with #4435 but should be easy to resolve. If your ready to publish I think it's good to go.

@DylanGuedes DylanGuedes marked this pull request as ready for review October 8, 2021 20:43
@DylanGuedes
Copy link
Contributor Author

DylanGuedes commented Oct 8, 2021

@DylanGuedes this looks good, will conflict with #4435 but should be easy to resolve. If your ready to publish I think it's good to go.

Awesome! I added an entry to the CHANGELOG and to the upgrading guide so now it is ready for review. :D

edit: and yeah, once #4435 gets merged I'll rewrite the tests on this one

@DylanGuedes DylanGuedes force-pushed the use-inmemory-as-default-kvstore branch from 58ff6bc to b1a43a0 Compare October 8, 2021 21:02
Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Dylan!

Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super nice! 🎉 Thanks @DylanGuedes LGTM!

@dannykopping
Copy link
Contributor

@DylanGuedes I'll merge this once the conflicts have been resolved

@kavirajk
Copy link
Contributor

Looks like @slim-bean have some opinion on it. We will hold off the merge!

@DylanGuedes DylanGuedes force-pushed the use-inmemory-as-default-kvstore branch 3 times, most recently from 91df8e7 to 04e0dbb Compare October 13, 2021 12:58
- Currently, the default KVstore is consul. This changes it to inmemory
to make it easier for new users to run the project without dependencies.
- Refactor default flag value tests. Now, it will check for a populated
map instead of checking for flags during iteration.
@DylanGuedes DylanGuedes force-pushed the use-inmemory-as-default-kvstore branch from 04e0dbb to d0a1a9b Compare October 13, 2021 13:12
@DylanGuedes
Copy link
Contributor Author

Note: I rewrote the commit history because it was becoming weird after rebasing with the latest changes.

Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@slim-bean slim-bean merged commit d3d63e1 into grafana:main Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants